Prompt users on instance operations#2547
Open
cjreynol wants to merge 8 commits into
Open
Conversation
cjreynol
force-pushed
the
prompt_for_instance_operations
branch
2 times, most recently
from
4e8b947 to
0367db2
Compare
cjreynol
force-pushed
the
prompt_for_instance_operations
branch
4 times, most recently
from
f496d80 to
a294e4e
Compare
They are not used. Bug: 511316553
The one location it was accessed was either to verify a value is valid, or using the result from the environment lookup was passed over. The flags are still available, so no functionality is lost with the removal. Bug: 511316553
Specifically, about the various cases it falls back to. The previous version was implicitly relying upon constraints enforced by different helpers and potentially ignoring "failed" `Result` returns. This version expects the operations to all succeed, then acts on those constraints locally. Bug: 511316553
To behave like `FindGroups`, where it allows multiple results to be returned. This will be used to rewrite `selector.cpp::SelectInstance` in the same way as `SelectGroup`. Bug: 511316553
In the same fashion as the `SelectGroup` rewrite, implement `SelectInstance` to be explicit about the various cases and fallbacks. This rewrite also stubs out instance-prompting, though it is not yet implemented. That will be in a follow-up commit, and technically this implementation is just as capable as the previous version. It just fails with a different error message. Bug: 511316553
The new helpers will be able to be reused for instance prompting. This version separates the prompting for a selection label from the display and looking up the group associated with that label. It also adds prompts to the user to make it clearer what label is expected, and catches different error cases separately. One slight downgrade for this version is it removes the functionality to type in a group name directly to select it. However, that functionality was not clearly advertised and unlikely to be faster than typing a numeric label except in cases that seem unlikely for the majority of users. Bug: 511316553
It reuses the group selection logic, then has the user pick an instance within that group. Bug: 511316553
Bug: 511316553
cjreynol
force-pushed
the
prompt_for_instance_operations
branch
from
a294e4e to
1131fb0
Compare
cjreynol
added
kokoro:force-run
GoogleCuttlefishTesterBot
removed
kokoro:run
jemoreira
reviewed
Jun 4, 2026
| char instance_idx = 'a'; | ||
| for (const auto& instance : group.Instances()) { | ||
| fmt::print(ss, " <{}> {}-{} (id : {})\n", instance_idx++, | ||
| group.GroupName(), instance.name(), instance.id()); |
Member
There was a problem hiding this comment.
nit: Printing the instance names alongside the group name makes it easier for the user to recognize each group. Especially with auto-generated group names.
Comment on lines
+141
to
+145
| if (filter.Empty()) { // try to default | ||
| groups = CF_EXPECT(instance_manager.FindGroups({})); | ||
| } else { | ||
| groups = CF_EXPECT(instance_manager.FindGroups(filter)); | ||
| } |
Member
There was a problem hiding this comment.
Isn't this equivalent to groups = CF_EXPECT(instance_manager.FindGroups(filter));?
| } | ||
| CF_EXPECT(isatty(0), | ||
| "Multiple groups found. Narrow the selection with selector " | ||
| "arguments or run in an interactive terminal."); |
Member
There was a problem hiding this comment.
nit: Running in an interactive terminal is not likely a good option when this error triggers. Drop that part of the message.
| Result<std::vector<std::pair<LocalInstance, LocalInstanceGroup>>> | ||
| InstanceDatabase::FindInstances(const Filter& filter) const { | ||
| CF_EXPECT_LE(filter.instance_names.size(), 1u, | ||
| "Can't find single instance when multiple names specified: " |
Member
There was a problem hiding this comment.
The function name indicates it's looking for multiple instances, this shouldn't be an error.
| } | ||
| Result<std::pair<LocalInstance, LocalInstanceGroup>> FindInstanceWithGroup( | ||
| const Filter& filter) const; | ||
| Result<std::vector<std::pair<LocalInstance, LocalInstanceGroup>>> |
Member
There was a problem hiding this comment.
If multiple instances from the same group match, this will return multiple copies of the same group. Consider changing the return type to vector<pair<Group, vector<Instance>>> or map<Group, vector<Instance>>.
Comment on lines
-37
to
-46
| Result<LocalInstanceGroup> GetDefaultGroup( | ||
| const InstanceManager& instance_manager) { | ||
| const std::vector<LocalInstanceGroup> all_groups = | ||
| CF_EXPECT(instance_manager.FindGroups({})); | ||
| CF_EXPECTF(all_groups.size() == 1, | ||
| "There are {} instance groups, unable to pick one", | ||
| all_groups.size()); | ||
| return all_groups.front(); | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
nit: This seems to be in the wrong commit
Comment on lines
+119
to
+120
| std::string menu = GroupDisplay(groups, DisplayBehavior::LabelGroup); | ||
| std::cout << menu; |
Member
There was a problem hiding this comment.
nit: std::out << GroupDisplay(...);
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When an instance-focused operation (eg
logs) is not given selectors or enough selectors, then havecvdprompt the user to select an instance rather than failing. This matches what we do for group-focused operations (remove).Bug: 511316553